-
Notifications
You must be signed in to change notification settings - Fork 27
[WIP] Add a logger #1124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add a logger #1124
Conversation
cd0e345
to
9c475df
Compare
Plugin build for 803d75f is ready 🛎️!
Note You can preview the changes in the Playground |
9c475df
to
574bade
Compare
71664d0
to
ae65e5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive logging functionality to Feedzy RSS Feeds, introducing a structured logging system to help debug import issues and monitor system health.
- Implements a new JSON-based logger with configurable log levels (debug, info, warning, error, none)
- Adds a logs management interface in the settings with filtering, export, and email reporting capabilities
- Replaces legacy
themeisle_log_event
usage throughout the codebase with the new logging system
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
includes/admin/feedzy-rss-feeds-log.php | Core logging class implementing singleton pattern with file-based JSON logging |
includes/admin/feedzy-rss-feeds-task-manager.php | Manages scheduled tasks for log cleanup and email reporting |
includes/layouts/settings.php | Updated settings page to include logs configuration and viewer tab |
includes/layouts/feedzy-logs-viewer.php | New logs viewer interface with filtering and export functionality |
includes/layouts/feedzy-email-report.php | Email template for weekly error reports |
tests/test-log.php | Comprehensive unit tests for the logging functionality |
tests/e2e/specs/logger.spec.js | End-to-end tests for the logging interface |
includes/admin/feedzy-rss-feeds-import.php | Updated import process to use new logging system extensively |
css/settings.css | Styling for the new logs interface |
js/feedzy-setting.js | JavaScript for log management UI interactions |
|
||
if ( is_wp_error( $logs ) ) { | ||
printf( | ||
'<p>%$1s(%2$s)</p>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a syntax error in the printf function call. The format string has mismatched placeholders - it should be '%1$s (%2$s)' but currently shows '%$1s(%2$s)' with incorrect placeholder syntax.
'<p>%$1s(%2$s)</p>', | |
'<p>%1$s (%2$s)</p>', |
Copilot uses AI. Check for mistakes.
header( 'Expires: 0' ); | ||
header( 'Connection: Keep-Alive' ); | ||
header( 'Cache-Control: must-revalidate' ); | ||
header( 'Pragma: public' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses filesize()
directly without checking if the file exists, but earlier checks use $this->log_file_exists()
and $this->is_log_readable()
. This could cause a PHP warning if the file doesn't exist at this point due to race conditions.
header( 'Pragma: public' ); | |
header( 'Pragma: public' ); | |
if ( ! $this->log_file_exists() || ! $this->is_log_readable() ) { | |
return new WP_Error( 'no_logs', '', array( 'status' => 404 ) ); | |
} |
Copilot uses AI. Check for mistakes.
header( 'Cache-Control: must-revalidate' ); | ||
header( 'Pragma: public' ); | ||
header( 'Content-Length: ' . filesize( $this->filepath ) ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the filesize issue, readfile()
is called without checking if the file still exists. Consider adding a final file existence check before these file operations.
// Final check before reading the file. | |
if ( ! file_exists( $this->filepath ) || ! is_readable( $this->filepath ) ) { | |
return new WP_Error( 'no_logs', '', array( 'status' => 404 ) ); | |
} |
Copilot uses AI. Check for mistakes.
} elseif ( strpos( $feed_img_tag, '[#item_custom' ) !== false ) { | ||
$value = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The variable $value
is declared but may not be used when the business/personal conditions are false. Consider moving this declaration inside the conditional block or providing a default value that's more meaningful.
$value = ''; | |
$value = null; |
Copilot uses AI. Check for mistakes.
tests/test-image-import.php
Outdated
@@ -70,7 +76,7 @@ public function test_import_image_special_characters() { | |||
$import_errors = array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable $import_errors
is declared but never used in this test method. Since the logging system has changed, this variable appears to be leftover from the old implementation and should be removed.
Copilot uses AI. Check for mistakes.
@Soare-Robert-Daniel, I noticed two things so far:
![]() |
This is intentional. The file is for us to analyze, its scope is to be given to support, then posted into the Github issues.
Yes |
@Soare-Robert-Daniel, I have used this feed URL https://www.nasa.gov/feeds/podcasts/explorers-apollo and the full content imported without any issue but the log shows an error Full content URL response received. Could you please let me know why it appears? ![]() ![]() |
This was marked wrong as an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left a few suggestions.
public function register_endpoints() { | ||
register_rest_route( | ||
'feedzy/v1', | ||
'/logs/download', | ||
array( | ||
'methods' => 'GET', | ||
'callback' => array( $this, 'export_logs_endpoint' ), | ||
'permission_callback' => function () { | ||
return current_user_can( 'manage_options' ); | ||
}, | ||
) | ||
); | ||
register_rest_route( | ||
'feedzy/v1', | ||
'/logs/delete', | ||
array( | ||
'methods' => 'GET', | ||
'callback' => array( $this, 'delete_log_file_endpoint' ), | ||
'permission_callback' => function () { | ||
return current_user_can( 'manage_options' ); | ||
}, | ||
) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make the second route either a POST or a DELETE request as it is not a read operation.
public static function debug( $message, array $context = array() ) { | ||
self::log( self::DEBUG, $message, $context ); | ||
} | ||
|
||
/** | ||
* Log an info message. | ||
* | ||
* @since 5.1.0 | ||
* @param string $message The log message. | ||
* @param array<string, mixed> $context The log context. | ||
* @return void | ||
*/ | ||
public static function info( $message, array $context = array() ) { | ||
self::log( self::INFO, $message, $context ); | ||
} | ||
|
||
/** | ||
* Log a warning message. | ||
* | ||
* @since 5.1.0 | ||
* @param string $message The log message. | ||
* @param array<string, mixed> $context The log context. | ||
* @return void | ||
*/ | ||
public static function warning( $message, array $context = array() ) { | ||
self::log( self::WARNING, $message, $context ); | ||
} | ||
|
||
/** | ||
* Log an error message. | ||
* | ||
* @since 5.1.0 | ||
* @param string $message The log message. | ||
* @param array<string, mixed> $context The log context. | ||
* @return void | ||
*/ | ||
public static function error( $message, array $context = array() ) { | ||
self::log( self::ERROR, $message, $context ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be great to have a trace of the error by including the first parameter to be __FUNCTION__
specially if it's made for debugging so one can easily debug which method threw the error.
224275f
to
9110416
Compare
b7b6a7b
to
803d75f
Compare
Summary
global $themeisle_log_event;
to gather error messages per import uiWill affect visual aspect of the product
NO
Screenshots
Test instructions
Important
Test with and without PRO version: https://github.com/Codeinwp/feedzy-rss-feeds-pro/pull/927
Check before Pull Request is ready:
Closes https://github.com/Codeinwp/feedzy-rss-feeds-pro/issues/845